Remove the old log* style constructors - #1278
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the legacy ANNError::log_* constructors and migrates call sites across the workspace to the newer ANNError::new(...) and ANNError::from_display(...) paths, aligning error creation with deferred formatting and more structured error propagation.
Changes:
- Deleted the
ANNError::log_*compatibility constructors and updated error docs/tests accordingly. - Updated call sites across
diskann,diskann-providers,diskann-disk,diskann-bftree, and tools/bench crates to constructANNErrorvianew(...)(typed errors) orfrom_display(...)(display-only errors). - Introduced an internal display-wrapper to allow carrying display values without eager string formatting.
Reviewed changes
Copilot reviewed 60 out of 60 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| diskann/src/utils/vector_repr.rs | Convert log_index_error(...) usage to ANNError::new(ANNErrorKind::IndexError, err) for typed error propagation. |
| diskann/src/utils/async_tools.rs | Replace legacy async error constructor with ANNError::new(ANNErrorKind::AsyncError, err). |
| diskann/src/graph/search/paged.rs | Replace paged-search parameter validation errors with ANNError::from_display(...). |
| diskann/src/graph/index.rs | Migrate async task/mutex error creation away from log_* constructors. |
| diskann/src/error/ranked.rs | Update tests and conversions to use ANNError::new(...) / from_display(...) instead of legacy constructors. |
| diskann/src/error/ann_error.rs | Remove log_* constructors; add from_display(...) helper and DisplayError wrapper. |
| diskann-tools/src/utils/search_index_utils.rs | Replace log_index_error(format_args!(...)) patterns with from_display(..., format!(...)). |
| diskann-providers/src/utils/sampling.rs | Replace invalid format / bounds errors with from_display(...). |
| diskann-providers/src/utils/rayon_util.rs | Convert thread-pool build errors to from_display(...). |
| diskann-providers/src/utils/medoid.rs | Replace legacy index errors (dimension, empty iterators, missing medoid) with from_display(...). |
| diskann-providers/src/utils/file_util.rs | Update vector-length validation errors to from_display(...). |
| diskann-providers/src/storage/pq_storage.rs | Replace PQ/index config errors with from_display(...) and preserve existing message structure. |
| diskann-providers/src/storage/index_storage.rs | Convert index save precondition failures to from_display(...). |
| diskann-providers/src/storage/bin.rs | Update metadata/vector dimension validation errors to from_display(...). |
| diskann-providers/src/model/pq/views.rs | Convert PQ view bridging errors to ANNError::new(PQError, ...) for typed errors. |
| diskann-providers/src/model/pq/strided.rs | Convert PQ strided bridging errors to ANNError::new(PQError, ...) for typed errors. |
| diskann-providers/src/model/pq/pq_construction.rs | Replace PQ parameter validation and quantization formatting errors with from_display(...). |
| diskann-providers/src/model/pq/generate_pivot_arguments.rs | Migrate pivot-argument errors to from_display(...). |
| diskann-providers/src/model/pq/fixed_chunk_pq_table.rs | Replace PQ table construction/validation errors with from_display(...). |
| diskann-providers/src/model/pq/distance/dynamic.rs | Replace dimension mismatch error creation with from_display(...). |
| diskann-providers/src/model/graph/provider/async_/simple_neighbor_provider.rs | Update index error construction for start-point validation to from_display(...). |
| diskann-providers/src/model/graph/provider/async_/memory_vector_provider.rs | Replace bounds/dimension validation errors with from_display(...). |
| diskann-providers/src/model/graph/provider/async_/memory_quant_vector_provider.rs | Replace bounds/dimension validation errors with from_display(...). |
| diskann-providers/src/model/graph/provider/async_/inmem/spherical.rs | Replace SQ error construction with ANNError::new(SQError, err) for typed errors. |
| diskann-providers/src/model/graph/provider/async_/inmem/scalar.rs | Replace SQ error construction with ANNError::new(SQError, err) for typed errors. |
| diskann-providers/src/model/graph/provider/async_/inmem/provider.rs | Replace async index/start-point validation errors with from_display(...). |
| diskann-providers/src/model/graph/provider/async_/fast_memory_vector_provider.rs | Replace bounds/dimension validation errors with from_display(...). |
| diskann-providers/src/model/graph/provider/async_/fast_memory_quant_vector_provider.rs | Replace bounds/dimension validation errors with from_display(...). |
| diskann-providers/src/model/graph/provider/async_/experimental/multi_pq_async.rs | Replace multiple legacy errors (table build, bounds, poison) with from_display(...). |
| diskann-providers/src/model/graph/provider/async_/common.rs | Replace legacy index/async errors with from_display(...). |
| diskann-providers/src/common/minmax_repr.rs | Replace MinMax conversion error formatting with from_display(...). |
| diskann-disk/src/utils/partition.rs | Replace dimension mismatch error with from_display(...). |
| diskann-disk/src/utils/math_util.rs | Replace various validation/overflow errors with from_display(...). |
| diskann-disk/src/utils/kmeans.rs | Replace PQ/KMeans cancellation/validation errors with from_display(...). |
| diskann-disk/src/storage/quant/pq/pq_generation.rs | Replace PQ compressor/table errors with from_display(...). |
| diskann-disk/src/storage/quant/pq/pq_dataset.rs | Replace PQ dataset construction/bounds errors with from_display(...). |
| diskann-disk/src/storage/quant/generator.rs | Replace PQ generator precondition failures with from_display(...). |
| diskann-disk/src/storage/disk_index_writer.rs | Replace index config/reader validation errors with from_display(...). |
| diskann-disk/src/storage/cached_reader.rs | Replace EOF/bounds read errors with from_display(...). |
| diskann-disk/src/search/provider/disk_vertex_provider.rs | Replace vertex-data/serde errors with from_display(...) (and keep existing message structure). |
| diskann-disk/src/search/provider/disk_vertex_provider_factory.rs | Replace allocation/strategy validation errors with new(...) / from_display(...). |
| diskann-disk/src/search/provider/disk_sector_graph.rs | Replace allocation/validation errors with new(...) / from_display(...). |
| diskann-disk/src/search/provider/aligned_file_reader/reader/windows.rs | Replace IO error creation with ANNError::new(IOError, err). |
| diskann-disk/src/search/provider/aligned_file_reader/reader/linux.rs | Replace IO/push errors with ANNError::new(..., err). |
| diskann-disk/src/search/provider/aligned_file_reader/aligned_read.rs | Replace alignment errors with from_display(...). |
| diskann-disk/src/search/pq/pq_scratch.rs | Replace allocation and dimension mismatch errors with new(...) / from_display(...). |
| diskann-disk/src/data_model/graph_metadata.rs | Replace parse-slice error creation with from_display(...) using legacy-equivalent formatting. |
| diskann-disk/src/data_model/graph_layout_version.rs | Replace parse-slice error creation with from_display(...) using legacy-equivalent formatting. |
| diskann-disk/src/data_model/graph_header.rs | Replace header error conversion with from_display(...). |
| diskann-disk/src/data_model/cache.rs | Replace cache validation errors with from_display(...). |
| diskann-disk/src/build/configuration/disk_index_build_parameter.rs | Replace index config parameter errors with from_display(...) preserving message semantics. |
| diskann-disk/src/build/builder/tokio.rs | Replace tokio runtime build failures with from_display(...). |
| diskann-disk/src/build/builder/quantizer.rs | Replace invalid SQ config error with from_display(...) preserving prior message structure. |
| diskann-disk/src/build/builder/build.rs | Replace thread-count / join / poison errors with from_display(...). |
| diskann-bftree/src/vectors.rs | Replace index errors in BF-tree vector provider with from_display(...). |
| diskann-bftree/src/quant.rs | Replace SQ/index errors with new(...) / from_display(...). |
| diskann-bftree/src/provider.rs | Replace multiple async/index errors (start points, params serde, metric parse) with from_display(...). |
| diskann-bftree/src/neighbors.rs | Replace BF-tree neighbor provider validation errors with from_display(...). |
| diskann-bftree/src/lib.rs | Replace transient escalation formatting with from_display(...). |
| diskann-benchmark/src/inputs/save_and_load.rs | Replace index config error with from_display(...) and preserve prior message semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if num_nodes_to_cache == 0 { | ||
| ANNError::log_index_error( | ||
| ANNError::from_display(diskann::ANNErrorKind::IndexError, | ||
| "num_nodes_to_cache should be greater than 0 for StaticCacheWithBfsNodes caching strategy", | ||
| ); | ||
| } |
| struct LocalError(tokio::task::JoinError); | ||
|
|
||
| ANNError::log_async_error(LocalError(err)) | ||
| ANNError::from_display(crate::ANNErrorKind::AsyncError, LocalError(err)) |
| pub struct DisplayError<D>(D); | ||
|
|
||
| impl<D> DisplayError<D> { | ||
| pub fn new(display: D) -> Self { | ||
| Self(display) | ||
| } | ||
| } |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1278 +/- ##
==========================================
- Coverage 91.50% 90.32% -1.18%
==========================================
Files 498 497 -1
Lines 95522 95527 +5
==========================================
- Hits 87407 86286 -1121
- Misses 8115 9241 +1126
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
We had an unfortunate situation where I was working on a similar PR simultaneously in #1282 😢. |
Remove `ANNErrorKind` from `ANNError`, removing all the old `log_` constructors and slimming the constructors to `new`, `message`, and `context. Since `diskann-disk` leans pretty heavily to using the kinds in its tests, a new `diskann_disk::error::Error` type is introduces along with a `diskann_error!` macro to largely preserve test structure and error kind tagging. See the review notes for more information. # Why The error kind enum is largely unused at this point, is not applied consistently, and complicates the API for `ANNError`. It's time to remove it and bring the size of `ANNError` down to 8-bytes. # Review Order **`diskann`** - `error/ann_error.rs`: Removal of the `ANNErrorKind` enum and the `log_*` style constructors. As a bonus, I added a `convert_error!` macro (happy to bike-shed) to stamp out simple conversion cases while preserving `track_caller`. The rest of the changes in `diskann` are removing the `ANNErrorKind` and switching over to the `convert_error!` macro. **`diskann-tools`/`diskann-providers`/`diskann-garnet`/`diskann-label-filter`/`diskann-bftree`/`diskann-benchmark-core`/`diskann-benchmark`** These mainly consist of updating the constructors and conversions. Note that where possible (and easily achievable), I use the [moving form of `lazy_format!`](https://github.com/microsoft/DiskANN/blob/e197b434912f7b13c64f1d89785f58ba7d5de22d/diskann-utils/src/lazystring.rs#L56-L74) to avoid eager string formatting. **`diskann-disk` This is the perhaps a more controversial change. `diskann-disk`'s tests lean heavily into the error kind and `log*` style constructors. I tried to preserve as much of the original behavior as possible by introducing an error type `diskann_disk::error::Error` consisting of a string-message an a `diskann_disk::error::ErrorKind` enum. A macro `diskann_disk::error::diskann_error!` helps embed this new `Error` inside of an `ANNError`. While this goes against the ethos of "don't have crate level errors", here is my rationale: * By using the `log_*` constructors, `diskann_disk`'s error types are already fundamentally tagged string messages. * Introducing an `Error` type means we can still check the `Kind` enum by downcasting the resulting `ANNError` down to the original `Error`. * Since the existing `diskann_disk` code is already doing eager string formatting/allocating at its error construction callsites, this isn't introduction a regression. Indeed, I've added some tricks to avoid allocating when constructing `diskann_disk::error::Error` from string literals, which is fairly common. * Moving the kind tag to `diskann_disk` allows the disk index to add error types without disrupting `ANNError`. # Alternatives * This is an alternative to #1278 that I was unfortunately working on in parallel. * One functionality we lost with this is the ability to recover the inner error types from the tag. Theoretically, the tag type could be used by third-party crates to guide the proper type for a downcast. In practice, no one is doing this. --------- Co-authored-by: Mark Hildebrand <mhildebrand@microsoft.com>
Reference Issues/PRs
Fixes #1003.
What does this implement/fix? Briefly explain your changes.
This change removes the legacy ANNError::log_* constructors and migrates all call sites to the newer ANNError::new(...) or ANNError::from_display(...) paths.
The main goal is to avoid the old eager string-formatting style and preserve more structured error handling where possible. Typed errors are now passed through ANNError::new(ANNErrorKind::..., err), while display-only legacy paths use ANNError::from_display(...) as a transitional wrapper. The legacy API documentation and direct tests for the removed log_* constructors were also cleaned up.
Any other comments?